feat: custom error types. #162

jvanz · 2023-11-21T17:33:20Z

Description

Add custom error types for all errors generated in the library.

jvanz · 2023-11-22T15:18:54Z

@kubewarden/kubewarden-developers THIS IS NOT READY. I would like to refine the error names and refactor the tests to follow our latest standards. ;)

jvanz · 2023-11-23T22:00:51Z

@kubewarden/kubewarden-developers THIS IS NOT READY. I would like to refine the error names and refactor the tests to follow our latest standards. ;)

@kubewarden/kubewarden-developers I've decided to mark this PR as ready for review now. I'll open another one fixing the tests. Just to avoid mixing two different fixes in a single PR

flavio · 2023-11-24T07:58:55Z

@kubewarden/kubewarden-developers I've decided to mark this PR as ready for review now. I'll open another one fixing the tests. Just to avoid mixing two different fixes in a single PR

From what I can see, only the Windows tests are broken and they are broken because of the changes introduced by this PR. Do not open a new PR, but rather fix the broken windows tests over there.
We can still review the PR in the meantime, just push the windows fixes into a dedicated commit.

flavio

Overall LGTM, I left some comments around

flavio · 2023-11-24T08:06:28Z

src/https.rs

+    type Error = FetcherErrors;

-    fn try_from(certificate: &Certificate) -> Result<Self> {
+    fn try_from(certificate: &Certificate) -> errors::Result<Self> {


Having defined the return type to be errors::Result (which implies Result<Self, FetcherErrors> I think we can void to define the type alias on line 22.

Nops, it's not possible. I've tried that while I was changing this file. This is the error:

cargo fmt --all && make test cargo fmt --all -- --check cargo clippy -- -D warnings Checking policy-fetcher v0.8.1 (/home/jvanz/suse/policy-fetcher) error[E0046]: not all trait items implemented, missing: `Error` --> src/https.rs:21:1 | 21 | impl TryFrom<&Certificate> for reqwest::Certificate { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `Error` in implementation | = help: implement the missing item: `type Error = /* Type */;` For more information about this error, try `rustc --explain E0046`. error: could not compile `policy-fetcher` (lib) due to previous error

Cargo.toml

flavio · 2023-11-24T08:11:11Z

src/lib.rs

@@ -16,6 +16,7 @@ pub mod sources;
 pub mod store;
 pub mod verify;

+use crate::errors::FetcherErrors;


I think we should also use the Result type define inside of the errors crate, just to avoid typing errors::Result

But then we will start to do what we are trying to avoid while using anyhow. From our code conventions:

Qualify anyhow::Result rather than use anyhow::Result.

Not if we entirely remove anyhow as a dependency of our crate. I would aim for that

src/registry/mod.rs

flavio · 2023-11-24T08:33:25Z

src/store/path/windows.rs

@@ -42,7 +42,7 @@ pub fn encode_filename(filename: &str) -> String {
 }

 /// Decode a path that was transformed with `encode_path`.
-pub fn decode_path<P: AsRef<Path>>(path: P) -> Result<PathBuf> {
+pub fn decode_path<P: AsRef<Path>>(path: P) -> errors::Result<PathBuf> {


According to the GH action, line 61 below is the one causing the windows tests to fail. Seems an easy fix :)

We are not handling the BASE64_ENGINE.decode error

src/verify/config.rs

flavio · 2023-11-24T08:37:58Z

src/verify/mod.rs

        if let Err(e) = Reference::try_from(image_name) {
-            return Err(anyhow!(
-                "Verification only works with OCI images: Not a valid oci image {}",
-                e
-            ));
+            return Err(FetcherErrors::InvalidOCIImageReferenceError(e));
        }


Not on you because it was already there, but I think we can write this in a more idiomatic way:

Reference::try_from(image_name).map_err(FetcherErrors::InvalidOCIImageReferenceError)?;

flavio · 2023-11-24T08:39:38Z

src/verify/mod.rs

-            "Verification only works with OCI images: Not a valid oci image {}",
-            e
-        ));
+        return Err(FetcherErrors::InvalidOCIImageReferenceError(e));


Same here, I think we can rewrite that with map_err()?

src/errors.rs

fabriziosestito

Thanks for taking care of this!
I've noticed that flatting errors of different modules in the same enum could introduce a problem.
For instance, if the std::fs::read(&self.local_path) in Policy::digest returns an error, the ? will convert the std::io::Error in the first available #[from] std::io::Error in the FetcherErrors enum.
In this case digest will return a CannotReadCertificateError variant.
You can check this by adding a small test to the policy module:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_digest_no_file() {
        let p = Policy {
            uri: "test.txt".to_string(),
            local_path: PathBuf::from("test.txt"),
        };

        let result = p.digest();
        assert!(matches!(
            result,
            Err(crate::FetcherErrors::CannotReadCertificateError(_))
        ));
    }
}

We could instead have separate error enums or structs in each module, depending on the module has to deal with one or more than one error, and then use those "inner" errors inside the FetcherErrors enum with the transparent macro, such as:

pub enum FetcherError {
   #[error(transparent)]
   PolicyError(#[from] crate::policy:DigestError)
  #[error(transparent)]
   CertifcateError(#[from] crate::CertficateError)
   ....
}

In the example above we'll need to introduce the DigestError directly in the policy module and change the code as follows:

#[derive(thiserror::Error, Debug)]
#[error("cannot retrieve path from uri: {err}")]
pub struct DigestError {
    #[from]
    err: std::io::Error,
}

impl Policy {
    pub fn digest(&self) -> Result<String, DigestError> {
        let d = Sha256::digest(std::fs::read(&self.local_path)?);
        Ok(format!("{:x}", d))
    }
}

where Result is the plain core::Result.

This way the error will be converted from the caller in the appropriate parent error, FetcherError in our case, and we avoid the error conversion clashing together. Also, we improve readability.

You can check a similar approach in the wild in this crate:
https://github.com/oxidecomputer/omicron/blob/3f702ef442a2cb6522684c8b4028bc8a8b11ed6d/illumos-utils/src/zfs.rs#L37

Also, I see that we could improve unit testing, by adding assertions on the type of errors in the errors path.

flavio · 2023-11-24T13:38:41Z

@fabriziosestito I didn't know about this approach. That looks way better than the one we use inside of other crates we contribute to!

Sounds like have to do some refactoring elsewhere :)

fabriziosestito

overall LGTM, some minor comments added.

Also, I've noticed we have some tests that check on errors.
Some example here but there could be more:

policy-fetcher/src/sources.rs

Line 303 in 30e22b0

match expected {

policy-fetcher/src/sources.rs

Line 286 in 30e22b0

let expected: Result<RawCertificate> = auth.try_into();

Could we update the test with an assertion on the type of the error instead of using is_err or even panicin?, such as

assert!(matches!(result, Err(NewError(_)))

src/errors.rs

src/https.rs

src/registry/errors.rs

src/sources.rs

src/store/errors.rs

src/verify/errors.rs

flavio · 2023-11-30T13:15:38Z

src/errors.rs

+#[derive(thiserror::Error, Debug)]
+#[error("invalid URL: {0}")]
+pub struct InvalidURLError(pub String);


Do we really need this error, can't we use the FetcherError::InvalidURLError?

This error exists because it is used in multiple modules. Thus, it's not necessary to duplicate it all over the place. And there is a wrapper around it in the FetcherError because the FetcherError is the type used in the Result returned by the lib to the callers.

flavio · 2023-11-30T13:16:32Z

src/errors.rs

+
+#[derive(thiserror::Error, Debug)]
+#[error("{0}")]
+pub struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error);


This is not "wrapped" by the FetcherError enum, isn't it?

It's not. But this error exists because it used in multiple modules. Those modules errors (SourceError and VerifyError) wraps this error.

Shouldn't we drop the pub attribute, or turn that into a pub(crate) if needed?

This will cause warning while compiling because we will use a more private type (FailedToParseYamlDataError) in more public ones (SourceError and RegistryError). As far as I know, we still want this types public. Therefore, I would like to avoid this warnings. To more context about the warning:

warning: type `FailedToParseYamlDataError` is more private than the item `SourceError::FailedToParseYamlDataError::0` --> src/sources.rs:27:40 | 27 | FailedToParseYamlDataError(#[from] FailedToParseYamlDataError), | ^^^^^^^^^^^^^^^^^^^^^^^^^^ field `SourceError::FailedToParseYamlDataError::0` is reachable at visibility `pub` | note: but type `FailedToParseYamlDataError` is only usable at visibility `pub(crate)` --> src/errors.rs:33:1 | 33 | pub(crate) struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ = note: `#[warn(private_interfaces)]` on by default warning: type `FailedToParseYamlDataError` is more private than the item `VerifyError::FailedToParseYamlDataError::0` --> src/verify/errors.rs:35:40 | 35 | FailedToParseYamlDataError(#[from] FailedToParseYamlDataError), | ^^^^^^^^^^^^^^^^^^^^^^^^^^ field `VerifyError::FailedToParseYamlDataError::0` is reachable at visibility `pub` | note: but type `FailedToParseYamlDataError` is only usable at visibility `pub(crate)` --> src/errors.rs:33:1 | 33 | pub(crate) struct FailedToParseYamlDataError(#[from] pub serde_yaml::Error); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: `policy-fetcher` (lib) generated 2 warnings

src/sources.rs

src/store/errors.rs

src/policy.rs

tests/store.rs

Add custom error types for all errors generated in the library. Signed-off-by: José Guilherme Vanz <[email protected]>

jvanz · 2023-12-07T14:35:17Z

@flavio we've decide (during Today's daily call ) to merge this because during the last daily call you did not seems against to it after discussing your last comment. Please, let us know if you want to change something. I'll be glad to do it.

jvanz self-assigned this Nov 21, 2023

jvanz force-pushed the this-error branch 3 times, most recently from 69bf318 to 25bb3ea Compare November 22, 2023 13:52

jvanz force-pushed the this-error branch 2 times, most recently from 55bdbc8 to bb605e3 Compare November 23, 2023 21:59

jvanz marked this pull request as ready for review November 23, 2023 22:00

jvanz requested a review from a team as a code owner November 23, 2023 22:00

flavio requested changes Nov 24, 2023

View reviewed changes

fabriziosestito requested changes Nov 24, 2023

View reviewed changes

jvanz force-pushed the this-error branch from bb605e3 to b1966a7 Compare November 28, 2023 21:45

jvanz requested review from flavio and fabriziosestito November 29, 2023 13:18

jvanz force-pushed the this-error branch 4 times, most recently from 2e32d20 to d022a18 Compare November 29, 2023 13:49

fabriziosestito requested changes Nov 29, 2023

View reviewed changes

jvanz force-pushed the this-error branch 4 times, most recently from 4d3a09b to 46ce05d Compare November 30, 2023 12:33

flavio reviewed Nov 30, 2023

View reviewed changes

jvanz force-pushed the this-error branch 2 times, most recently from 9d8ce64 to 3ab1eb0 Compare December 1, 2023 13:23

jvanz requested a review from flavio December 1, 2023 13:33

jvanz requested a review from fabriziosestito December 1, 2023 13:33

jvanz force-pushed the this-error branch from 3ab1eb0 to e1d5f23 Compare December 1, 2023 13:37

fabriziosestito requested changes Dec 4, 2023

View reviewed changes

src/sources.rs Outdated Show resolved Hide resolved

src/sources.rs Outdated Show resolved Hide resolved

src/store/errors.rs Outdated Show resolved Hide resolved

src/policy.rs Outdated Show resolved Hide resolved

jvanz requested a review from fabriziosestito December 5, 2023 13:26

jvanz force-pushed the this-error branch from e1d5f23 to ea2f438 Compare December 5, 2023 13:26

fabriziosestito reviewed Dec 5, 2023

View reviewed changes

tests/store.rs Show resolved Hide resolved

jvanz requested a review from fabriziosestito December 6, 2023 01:12

fabriziosestito approved these changes Dec 6, 2023

View reviewed changes

feat: custom error types.

0f7fc34

Add custom error types for all errors generated in the library. Signed-off-by: José Guilherme Vanz <[email protected]>

jvanz force-pushed the this-error branch from ea2f438 to 0f7fc34 Compare December 7, 2023 14:17

jvanz merged commit 9c90873 into kubewarden:main Dec 7, 2023
7 checks passed

jvanz deleted the this-error branch December 7, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom error types. #162

feat: custom error types. #162

jvanz commented Nov 21, 2023

jvanz commented Nov 22, 2023

jvanz commented Nov 23, 2023 •

edited

Loading

flavio commented Nov 24, 2023

flavio left a comment

flavio Nov 24, 2023

jvanz Nov 24, 2023

flavio Nov 24, 2023

jvanz Nov 24, 2023

flavio Nov 24, 2023

flavio Nov 24, 2023

flavio Nov 24, 2023

flavio Nov 24, 2023

fabriziosestito left a comment •

edited

Loading

flavio commented Nov 24, 2023

fabriziosestito left a comment •

edited

Loading

flavio Nov 30, 2023

jvanz Dec 1, 2023

flavio Nov 30, 2023

jvanz Dec 1, 2023

flavio Dec 5, 2023

jvanz Dec 6, 2023

jvanz commented Dec 7, 2023 •

edited

Loading

feat: custom error types. #162

feat: custom error types. #162

Conversation

jvanz commented Nov 21, 2023

Description

jvanz commented Nov 22, 2023

jvanz commented Nov 23, 2023 • edited Loading

flavio commented Nov 24, 2023

flavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito left a comment • edited Loading

Choose a reason for hiding this comment

flavio commented Nov 24, 2023

fabriziosestito left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvanz commented Dec 7, 2023 • edited Loading

jvanz commented Nov 23, 2023 •

edited

Loading

fabriziosestito left a comment •

edited

Loading

fabriziosestito left a comment •

edited

Loading

jvanz commented Dec 7, 2023 •

edited

Loading